fix(csharp): set BUFFER_LENGTH to null in GetColumns result; re-enable CanGetColumnsExtended for SEA (PECO-3008)#444
Conversation
…e-enable CanGetColumnsExtended for SEA (PECO-3008) JDBC spec defines BUFFER_LENGTH as unused — it should always be null. FlatColumnsResultBuilder was computing non-null values for numeric types (INT=4, BIGINT=8, etc.) via ColumnMetadataHelper.GetBufferLength, causing CanGetColumnsExtended to fail for the SEA three-calls fallback path while Thrift (which reads directly from the server and returns null) passed. CreateExtendedColumnsResult (DESC TABLE EXTENDED path) already returned null unconditionally; this change makes FlatColumnsResultBuilder consistent. Removes the Skip.If(Protocol == "rest") guard from CanGetColumnsExtended. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
BUFFER_LENGTH is a JDBC-compatibility column (ADBC's xdbc_buffer_length) defined as "not used", so it is consistently null across all paths — server-driven on Thrift and client-built on SEA. Reword the comment to reflect the ADBC/JDBC-compat framing rather than just "JDBC spec". Co-authored-by: Isaac
…by managed/Arrow APIs Expand the comment: BUFFER_LENGTH is ODBC's SQLColumns buffer-allocation hint (bytes to bind/fetch a column into a caller-owned C buffer). Managed APIs that own their buffers don't need it — JDBC marks it "not used", and ADBC even less so since Arrow results are self-describing (buffer sizes come from the type + data). Hence always null, on all paths. Co-authored-by: Isaac
eric-wang-1990
requested review from
Copilot and
msrathore-db
and removed request for
msrathore-db
There was a problem hiding this comment.
Pull request overview
This PR aligns the C# driver’s GetColumns/GetColumnsExtended metadata results across SEA (REST) and Thrift by making BUFFER_LENGTH consistently null, and re-enables the previously skipped SEA E2E test that validates GetColumnsExtended.
Changes:
- Update
FlatColumnsResultBuilderto always emitnullforBUFFER_LENGTHinstead of computing numeric byte sizes. - Re-enable
CanGetColumnsExtendedfor SEA by removing theProtocol == "rest"skip guard.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| csharp/src/FlatColumnsResultBuilder.cs | Makes BUFFER_LENGTH always null to match the Thrift path and the DESC TABLE EXTENDED path. |
| csharp/test/E2E/StatementTests.cs | Removes the SEA skip so CanGetColumnsExtended runs for REST/SEA again. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
msrathore-db
left a comment
msrathore-db
left a comment
There was a problem hiding this comment.
The BUFFER_LENGTH → AppendNull() change is correct and matches the Thrift / DESC TABLE EXTENDED paths. However, I tested this branch live against a SQL warehouse with protocol=rest, and removing Skip.If(Protocol == "rest") does not make CanGetColumnsExtended pass on SEA:
useDescTableExtended=true(DESC TABLE EXTENDED path) → passes.useDescTableExtended=false(theFlatColumnsResultBuilder3-call path this PR touches) → fails, on fields beyondBUFFER_LENGTH:SQL_DATA_TYPE: actual-6, expectednull.FlatColumnsResultBuilder.cs:92doessqlDataTypeBuilder.Append(info.ColType[i]), whereas the DESC path (DatabricksStatement.cs:1089) and Thrift build it all-null — same class of issue asBUFFER_LENGTH, one field over.IS_NULLABLE: PK columnc_intreturns"NO"on SEA's 3-call path vs the expected"YES"(thePK_IS_NULLABLE:YESplaceholder for the false variant).
I confirmed locally that nulling SQL_DATA_TYPE clears the first diff, but IS_NULLABLE still fails afterward — so the BUFFER_LENGTH fix is necessary but not sufficient.
Suggest either (a) reconciling SQL_DATA_TYPE and IS_NULLABLE in the SEA 3-call path before dropping the skip, or (b) scoping the skip to useDescTableExtended=false + SEA only.
Heads-up: if CI doesn't run the SEA matrix for this test, this will merge green while the SEA path it's meant to enable stays broken (the test fails whenever SEA is actually exercised).
Problem
CanGetColumnsExtendedwas skipped for SEA (Skip.If(Protocol == "rest")) because the SEA path returned differentBUFFER_LENGTHvalues than Thrift.Root cause:
FlatColumnsResultBuilder(used by the three-calls fallback path viaGetColumnsAsync) calledColumnMetadataHelper.GetBufferLengthwhich returned non-null values for numeric types (BOOLEAN=1, TINYINT=1, SMALLINT=2, INT=4, FLOAT=4, BIGINT=8, DOUBLE=8, TIMESTAMP=8, DATE=8, DECIMAL=computed). The expected test values and the Thrift path both returnnullfor all columns.The JDBC spec defines
BUFFER_LENGTHas an unused column — it should always benull.CreateExtendedColumnsResult(theDESC TABLE EXTENDEDpath) already returnednullunconditionally viabufferLengthBuilder.AppendNull().FlatColumnsResultBuilderwas inconsistent.Changes
FlatColumnsResultBuilder: replace theGetBufferLengthcall withAppendNull(), makingBUFFER_LENGTHconsistently null across all result paths (matchingCreateExtendedColumnsResultand Thrift).StatementTests.cs: remove theSkip.If(Protocol == "rest")guard fromCanGetColumnsExtended.Test Plan
CanGetColumnsExtendednow runs for both Thrift and SEA protocols, covering bothuseDescTableExtended=trueanduseDescTableExtended=false.Fixes PECO-3008
🤖 Generated with Claude Code